Added implementation on set_permissions_nofollow for all primary platforms#158168
Added implementation on set_permissions_nofollow for all primary platforms#158168asder8215 wants to merge 1 commit into
set_permissions_nofollow for all primary platforms#158168Conversation
|
r? @clarfonthey rustbot has assigned @clarfonthey. Use Why was this reviewer chosen?The reviewer was selected based on:
|
3411717 to
96bc9a1
Compare
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| pub fn set_perm_nofollow(path: &Path, perm: FilePermissions) -> io::Result<()> { | ||
| set_perm(path, perm) |
There was a problem hiding this comment.
Similar to UEFI, I would swap the method bodies here, so that set_perm defers to set_perm_nofollow plus a comment that says symlinks aren't supported.
|
@rustbot author CI failure appears to be a simple typo but I also left some feedback on other stuff to fix. Otherwise, implementation looks good, although I would add an extra comment on the tracking issue once this is merged to have some Windows folks double-check that opening all reparse points in this method doesn't do anything we don't want, since that technically includes more than just symlinks. It should work in pretty much all normal use cases, just want to double-check the edge cases before stabilisation. |
|
Reminder, once the PR becomes ready for a review, use |
96bc9a1 to
f9c87ed
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
f9c87ed to
feafe24
Compare
This comment has been minimized.
This comment has been minimized.
feafe24 to
b286acf
Compare
This comment has been minimized.
This comment has been minimized.
b286acf to
14df6fa
Compare
This comment has been minimized.
This comment has been minimized.
14df6fa to
7029495
Compare
|
Let's try this again... @bors try jobs=arm-android |
This comment has been minimized.
This comment has been minimized.
…=<try> Added implementation on `set_permissions_nofollow` for all primary platforms try-job: arm-android
|
@bors try (let's just do a full run at this point. might fail due to GHA being awful today, but worth a shot) |
This comment has been minimized.
This comment has been minimized.
…=<try> Added implementation on `set_permissions_nofollow` for all primary platforms
|
@bors try cancel I'll poke and see what I actually need to test. |
|
Try build cancelled. Cancelled workflows: |
|
@bors try jobs=dist-android Eh, let's just do this one too for now. Misunderstood how try worked. Then if that passes I'll add to the queue again. |
This comment has been minimized.
This comment has been minimized.
…=<try> Added implementation on `set_permissions_nofollow` for all primary platforms try-job: dist-android
|
@bors r+ rollup=iffy Multitarget code that has failed us before, but hopefully shouldn't this time… |
|
💔 I suspect this PR failed tests as part of a rollup After fixing the problem, consider running a try job for the failed job before re-approving. Link to failure: #159952 (comment) |
|
This pull request was unapproved. This PR was contained in a rollup (#159952), which was unapproved. |
There was a problem hiding this comment.
On Windows you can set attributes on symlinks (they're just normal files or directories).
There was a problem hiding this comment.
Got you. Does setting an attribute on symlinks do anything meaningful? I was reading up on MacOS and they also can set permission bits on symlinks, but they don't do anything meaningful.
|
@bors try jobs=x86_64-msvc-1 |
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 4e6ca17 failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
|
Okay, that confirms to me that Windows actually sets permissions on the |
…pported (windows, unix, uefi, etc.); modified the Unix implementation to use fchmodat instead of open + fchmod; clarified documentations for set_permissions_nofollow; added a test case for set_permissions_nofollow
|
@bors try jobs=x86_64-msvc-1 |
|
⌛ Trying commit b319483 with merge a576dd2… To cancel the try build, run the command Workflow: https://github.com/rust-lang/rust/actions/runs/30215028040 |
View all comments
For context, this PR is related to the tracking issue for
std::fs::set_permissions_nofollow. This PR does a few different things:std::fs::set_permissions_nofollow. On Windows, it usesOpenOptions::openwith the custom flagFILE_FLAG_OPEN_REPARSE_POINTenabled and then sets the permissions on the reparse point directly. All other platforms (hermit, motor, solid, uefi, etc.) defer to whatfs::set_permissionsdoes since symlinks aren't supported on those platforms, so they effectively do the same thing.fchmodatinstead of doing two separate calls (openand thenfchmodviaOpenOptions).fchmodinstead ofchmodunderneath the hood (as that's whatFile::set_permissions, which is not to be confused withfs::set_permissionsas that does usechmod, does underneath the hood), it actually had some incorrect documentation about the error returned byfchmodon symlinks. Instead ofio::ErrorKind::FilesystemLoop, it returnsio::ErrorKind::InvalidInput. You can read the documentation forfchmod.fchmodatdoes effectively the same thing as theopen+fchmodcombo, but it does return a different error,io::ErrorKind::Unsupported, that makes more sense. Regardless, I corrected the documentation forstd::fs::set_permissions_nofollowand added a lot more clarifying information.set_permissions_nofollowis operating correctly.cc @ChrisDenton and @lolbinarycat